Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin, mapping jobs with TPV: Fix a few speling mistakes #4559

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

torfinnnome
Copy link
Contributor

No description provided.

@torfinnnome torfinnnome requested a review from a team as a code owner November 28, 2023 17:22
@github-actions github-actions bot added the admin label Nov 28, 2023
@@ -270,7 +270,7 @@ We want our tool to run with more than one core. To do this, we need to instruct
> - env:
> - - name: LC_ALL
> - value: C
> - - name: SINGULARITY_CACHEDIR
> - - name: APPTAINER_CACHEDIR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These does not seem a spelling mistake. Can you please explain why are you making this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I should have made a separate PR for this. Throughout the rest of the training material, APPTAINER_ is used (and partly in this tutorial), so I assume it was just a mistake to not previously change this from SINGULARITY_.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If singularity is not used at all then the corresponding value on the next line also does not make sense and more changes are required I believe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these variables are interpreted by Galaxy, and must be SINGULARITY (mostly job conf, e.g. singularity_enabled)

Others are read by apptainer and can safely be APPTAINER. TMPDIR is one of those that isn't read by galaxy afaict (grepping release_23.1`)

$ ag CACHEDIR
doc/source/admin/container_resolvers.rst
169:For singularity, admins should also take care of the ``APPTAINER_CACHEDIR``

cachedir it seems is too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and in the Apptainer tutorial, both APPTAINER_CACHEDIR and APPTAINER_TMPDIR are used.

@hexylena
Copy link
Member

hexylena commented Dec 5, 2023

@torfinnnome can you find and replace the other occurrences of these vars? then tests will pass.

$ ag CACHEDIR --markdown -l
topics/admin/tutorials/job-destinations/tutorial.md
topics/admin/tutorials/apptainer/tutorial.md
topics/admin/tutorials/connect-to-compute-cluster/tutorial.md

@hexylena hexylena merged commit 90d7678 into galaxyproject:main Dec 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants